feat: snapshot serde #74
Conversation
Signed-off-by: Junwang Zhao <[email protected]>
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/iceberg/json_internal.cc:574
- Consider verifying that summary_json is an object before iterating over its items to avoid potential exceptions if the JSON structure does not match expectations.
for (const auto& [key, value] : summary_json.items()) {
src/iceberg/type_fwd.h:105
- [nitpick] Changing TableMetadata from a class to a struct may affect encapsulation; please confirm that this change is intentional and consistent with its usage elsewhere.
struct TableMetadata;
| } | ||
| } | ||
|
|
||
| Result<std::unique_ptr<Snapshot>> SnapshotFromJson(const nlohmann::json& json) { |
There was a problem hiding this comment.
I think we need to be consistent with the Java impl: https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/SnapshotParser.java. Specifically, we need to deal with cases where sequence number or summary is missing.
@Fokko Will it actually happen that a snapshot does not have summary (and thus operation is also missing)?
There was a problem hiding this comment.
From the spec, summary is required for v2 and v3 but optional for v1. So I believe the spec answers my question. We have to handle this.
There was a problem hiding this comment.
You're correct!
- For V1 the summary is optional.
- For V2/V3 the summary is required, and also the operation. Some writers produced some malformed metadata in the past. Instead of throwing an exception, we would it is an overwrite operation, since that's the most generic one.
There was a problem hiding this comment.
It sounds like we need to set operation to overwrite when summary is available but operation is missing. @zhjwpku
There was a problem hiding this comment.
Fixed, please take a look.
|
I've updated the PR to use |
zhjwpku
force-pushed
the
snapshot_ser_der
branch
from
d792f25 to
8847ca8
Compare
|
Thanks for working on this @zhjwpku, and thanks for the review @lidavidm, @yingcai-cy and @wgtmac 🙌 |
5 participants
No description provided.